Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SCHEMA] Mark CHANGES as optional #1151

Merged
merged 2 commits into from
Jul 20, 2022
Merged

[SCHEMA] Mark CHANGES as optional #1151

merged 2 commits into from
Jul 20, 2022

Conversation

yarikoptic
Copy link
Collaborator

@yarikoptic yarikoptic commented Jul 19, 2022 •
edited by effigies
Loading

Current wording at
https://bids-specification.readthedocs.io/en/stable/03-modality-agnostic-files.html#changes

### `CHANGES`

Version history of the dataset (describing changes, updates and corrections) MAY
be provided in the form of a `CHANGES` text file. This file MUST follow the
[CPAN Changelog convention](https://metacpan.org/pod/release/HAARG/CPAN-Changes-0.400002/lib/CPAN/Changes/Spec.pod).
The `CHANGES` file MUST be either in ASCII or UTF-8 encoding.

so it MAY be provided, not MUST. Alerted to by @satra in
dandi/dandi-cli#1055 (comment)

Closes #1127.

@yarikoptic
BF: schema - do not announce that CHANGES is required
246b35f 
Current wording at
https://bids-specification.readthedocs.io/en/stable/03-modality-agnostic-files.html#changes

	### `CHANGES`

	Version history of the dataset (describing changes, updates and corrections) MAY
	be provided in the form of a `CHANGES` text file. This file MUST follow the
	[CPAN Changelog convention](https://metacpan.org/pod/release/HAARG/CPAN-Changes-0.400002/lib/CPAN/Changes/Spec.pod).
	The `CHANGES` file MUST be either in ASCII or UTF-8 encoding.

so it MAY be provided, not MUST.  Alerted to by @satra in
dandi/dandi-cli#1055 (comment)
@yarikoptic yarikoptic added the schema Issues related to the YAML schema representation of the specification. Patch version release. label Jul 19, 2022
tsalo
tsalo approved these changes Jul 19, 2022
View reviewed changes
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@sappelhoff
Copy link
Member

closes #1127 ?

@yarikoptic yarikoptic mentioned this pull request Jul 19, 2022
Closed
@sappelhoff sappelhoff changed the title BF: schema - do not announce that CHANGES is required [SCHEMA] do not announce that CHANGES is required Jul 19, 2022
@effigies effigies changed the title [SCHEMA] do not announce that CHANGES is required [SCHEMA] Mark CHANGES as optional Jul 19, 2022
@yarikoptic
Copy link
Collaborator Author

closes #1127 ?

yes indeed apparently, thank you!

@sappelhoff
Copy link
Member

errors are due to #1000 which I merged a couple of minutes ago, I think

@yarikoptic
Copy link
Collaborator Author

oh - schemacode_ci is apparently red! need to discover now if I broke it or not. It is that day -- file bids-standard/maintenance-tools#11 with my intention.

@yarikoptic
Copy link
Collaborator Author

re CI failure -- not me -- recent commit 130aa76 in master was already failing them. Here is what I see from the con/tinuous dumps with regular grep on sample failing test :

datalad@smaug:/mnt/datasets/datalad/ci/bids-specification/2022/07$ git grep 'FAILED.*test_make_glossary' | grep 3.7
19/pr/1143/b82ae52/github-schemacode_ci-656-failed/1_ubuntu-latest with Python 3.7.txt:2022-07-19T14:37:26.4504200Z FAILED tools/schemacode/bidsschematools/tests/test_render.py::test_make_glossary
19/pr/1143/b82ae52/github-schemacode_ci-656-failed/ubuntu-latest with Python 3.7/6_Run tests.txt:2022-07-19T14:37:26.4504197Z FAILED tools/schemacode/bidsschematools/tests/test_render.py::test_make_glossary
19/pr/1151/246b35f/github-schemacode_ci-657-failed/1_ubuntu-latest with Python 3.7.txt:2022-07-19T14:47:18.4379692Z FAILED tools/schemacode/bidsschematools/tests/test_render.py::test_make_glossary
19/pr/1151/246b35f/github-schemacode_ci-657-failed/ubuntu-latest with Python 3.7/6_Run tests.txt:2022-07-19T14:47:18.4379674Z FAILED tools/schemacode/bidsschematools/tests/test_render.py::test_make_glossary
19/push/master/130aa76/github-schemacode_ci-655-failed/1_ubuntu-latest with Python 3.7.txt:2022-07-19T14:21:52.7698770Z FAILED tools/schemacode/bidsschematools/tests/test_render.py::test_make_glossary
19/push/master/130aa76/github-schemacode_ci-655-failed/ubuntu-latest with Python 3.7/6_Run tests.txt:2022-07-19T14:21:52.7698751Z FAILED tools/schemacode/bidsschematools/tests/test_render.py::test_make_glossary

and update of this page brings me in agreement with @sappelhoff on that in #1151 (comment)

@yarikoptic
Copy link
Collaborator Author

what is the policy on such trivial fixes - do they also need to stage for 5 days or so?

@sappelhoff
Copy link
Member

what is the policy on such trivial fixes - do they also need to stage for 5 days or so?

we don't have strict rules of what constitutes "trivial fixes" and how we deal with them, but usually a single review suffices and no "staging" for 5 days is needed.

@sappelhoff sappelhoff mentioned this pull request Jul 20, 2022
Merged
@codecov
Copy link

codecov bot commented Jul 20, 2022 •
edited
Loading

Codecov Report

Merging #1151 (c226811) into master (22b7db6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    bids-standard/bids-specification#1151   +/-   ##
=======================================
  Coverage   91.36%   91.36%           
=======================================
  Files          10       10           
  Lines        1042     1042           
=======================================
  Hits          952      952           
  Misses         90       90

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22b7db6...c226811. Read the comment docs.

@sappelhoff sappelhoff merged commit 4e196db into bids-standard:master Jul 20, 2022
@effigies effigies added the exclude-from-changelog This item will not feature in the automatically generated changelog label Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsalo tsalo tsalo approved these changes

@effigies effigies effigies approved these changes

@erdalkaraca erdalkaraca Awaiting requested review from erdalkaraca erdalkaraca is a code owner

Assignees
No one assigned
Labels
exclude-from-changelog This item will not feature in the automatically generated changelog schema Issues related to the YAML schema representation of the specification. Patch version release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SCHEMA] CHANGES has required: true despite it being optional
4 participants
@yarikoptic @sappelhoff @effigies @tsalo